Re: Add method to derive a wallet from PrivateKey#3063
Open
interc0der wants to merge 7 commits intoXRPLF:mainfrom
Open
Re: Add method to derive a wallet from PrivateKey#3063interc0der wants to merge 7 commits intoXRPLF:mainfrom
interc0der wants to merge 7 commits intoXRPLF:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds private-key-to-public-key derivation: new SigningScheme method to derive a KeyPair from a private key, secp256k1 and ed25519 implementations, a top-level Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
3 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
packages/ripple-keypairs/src/types.ts (1)
21-22: Clarify method contract and canonicalization expectationsPlease document the accepted formats (prefixes/lengths) for each algorithm and whether implementations must canonicalize the returned privateKey. This avoids ambiguity across backends and downstream callers.
Apply this diff to add TSDoc:
- deriveKeypairFromPrivateKey: (privateKey: HexString) => KeyPair + /** + * Derive a keypair from a private key. + * Implementations should accept: + * - secp256k1: '00' + 64-hex (66 chars) or raw 64-hex. + * - ed25519: 'ED' + 64-hex (66 chars). Raw 64-hex should be rejected to avoid ambiguity with secp256k1. + * + * Returned privateKey SHOULD be canonicalized with the correct prefix for the algorithm to ensure + * compatibility with downstream APIs (e.g., ed25519 signing requires the 'ED' prefix). + */ + deriveKeypairFromPrivateKey: (privateKey: HexString) => KeyPairpackages/ripple-keypairs/src/index.ts (1)
98-110: Simplify by delegating via getSigningSchemeThis avoids duplicating algorithm branches and keeps the implementation aligned with other APIs (sign/verify).
Apply this diff:
-function derivePublicKey(privateKey: string): string { - const algorithm = getAlgorithmFromPrivateKey(privateKey) - - if (algorithm === 'ecdsa-secp256k1') { - return secp256k1.deriveKeypairFromPrivateKey(privateKey).publicKey - } - // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition - if (algorithm === 'ed25519') { - return ed25519.deriveKeypairFromPrivateKey(privateKey).publicKey - } - throw new Error('Unknown signing scheme algorithm') -} +function derivePublicKey(privateKey: string): string { + const algorithm = getAlgorithmFromPrivateKey(privateKey) + const scheme = getSigningScheme(algorithm) + return scheme.deriveKeypairFromPrivateKey(privateKey).publicKey +}packages/xrpl/test/wallet/index.test.ts (1)
456-461: Consider asserting the error class/message to prevent false positivesRight now we only assert that an error is thrown. If the implementation throws a different error (e.g., Algorithm mismatch vs. invalid hex), these tests would still pass. When feasible, narrow this with a regex or specific error class (e.g., ValidationError), similar to other tests in this file.
Example:
assert.throws( () => Wallet.fromPrivateKey(...), /invalid|malformed|unknown signing scheme/i, )If the message is stable, prefer a precise regex.
Would you like me to align this to the actual error text thrown by derivePublicKey() so the tests are resilient?
Also applies to: 477-482
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/ripple-keypairs/src/index.ts(2 hunks)packages/ripple-keypairs/src/signing-schemes/ed25519/index.ts(1 hunks)packages/ripple-keypairs/src/signing-schemes/secp256k1/index.ts(1 hunks)packages/ripple-keypairs/src/types.ts(1 hunks)packages/xrpl/src/Wallet/index.ts(2 hunks)packages/xrpl/test/wallet/index.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/xrpl/src/Wallet/index.ts (2)
packages/xrpl/src/index.ts (1)
Wallet(12-12)packages/xrpl/src/errors.ts (1)
ValidationError(156-156)
🔇 Additional comments (2)
packages/xrpl/src/Wallet/index.ts (1)
20-20: Import looks correctImporting derivePublicKey where used is correct and consistent with the rest of the module.
packages/ripple-keypairs/src/index.ts (1)
123-124: Good addition to the public APIExporting derivePublicKey alongside deriveAddress/deriveNodeAddress makes sense and keeps xrpl Wallet usage minimal.
Comment on lines
+22
to
+42
| deriveKeypairFromPrivateKey(privateKey: string): { | ||
| privateKey: string | ||
| publicKey: string | ||
| } { | ||
| assert.ok( | ||
| privateKey.startsWith(ED_PREFIX) | ||
| ? privateKey.length === 66 | ||
| : privateKey.length === 64, | ||
| 'Invalid ed25519 private key length', | ||
| ) | ||
|
|
||
| const normalizedPrivateKey = privateKey.startsWith(ED_PREFIX) | ||
| ? privateKey.slice(2) | ||
| : privateKey | ||
|
|
||
| const buffer = Buffer.from(normalizedPrivateKey, 'hex') | ||
|
|
||
| const publicKey = ED_PREFIX + bytesToHex(nobleEd25519.getPublicKey(buffer)) | ||
| return { privateKey, publicKey } | ||
| }, | ||
|
|
Contributor
There was a problem hiding this comment.
Require ED prefix and avoid Buffer for isomorphic support
- ed25519 keys must carry the ED prefix across this codebase (sign() enforces length 66). Accepting raw 64-hex here is inconsistent and creates ambiguity with secp256k1 detection.
- Replace Buffer usage with hexToBytes for browser compatibility.
- Add strict hex validation.
Apply this diff within the selected range:
deriveKeypairFromPrivateKey(privateKey: string): {
privateKey: string
publicKey: string
} {
- assert.ok(
- privateKey.startsWith(ED_PREFIX)
- ? privateKey.length === 66
- : privateKey.length === 64,
- 'Invalid ed25519 private key length',
- )
-
- const normalizedPrivateKey = privateKey.startsWith(ED_PREFIX)
- ? privateKey.slice(2)
- : privateKey
-
- const buffer = Buffer.from(normalizedPrivateKey, 'hex')
-
- const publicKey = ED_PREFIX + bytesToHex(nobleEd25519.getPublicKey(buffer))
- return { privateKey, publicKey }
+ assert.ok(
+ privateKey.startsWith(ED_PREFIX) && privateKey.length === 66,
+ 'ed25519 private key must be 33 bytes including "ED" prefix',
+ )
+ const normalizedPrivateKey = privateKey.slice(2)
+ assert.ok(
+ /^[0-9a-fA-F]{64}$/.test(normalizedPrivateKey),
+ 'private key must be 32-byte hex',
+ )
+ const privBytes = hexToBytes(normalizedPrivateKey)
+ const publicKey =
+ ED_PREFIX + bytesToHex(nobleEd25519.getPublicKey(privBytes))
+ return { privateKey, publicKey }
},And update the import (outside this hunk):
// change this:
import { bytesToHex } from '@xrplf/isomorphic/utils'
// to this:
import { bytesToHex, hexToBytes } from '@xrplf/isomorphic/utils'🤖 Prompt for AI Agents
In packages/ripple-keypairs/src/signing-schemes/ed25519/index.ts around lines 22
to 42, enforce that private keys must include the ED_PREFIX (no accepting 64-hex
raw keys), validate the hex string strictly (only [0-9a-fA-F] and exact length
after prefix), convert the normalized hex to bytes using hexToBytes instead of
Buffer for isomorphic/browser support, and return the publicKey prefixed with
ED_PREFIX; also update the module import (outside this hunk) to import
hexToBytes along with bytesToHex as requested.
Comment on lines
+33
to
+52
| deriveKeypairFromPrivateKey(privateKey: string): { | ||
| privateKey: string | ||
| publicKey: string | ||
| } { | ||
| assert.ok( | ||
| (privateKey.length === 66 && privateKey.startsWith(SECP256K1_PREFIX)) || | ||
| privateKey.length === 64, | ||
| 'Invalid private key length or format', | ||
| ) | ||
| const normalizedPrivateKey = | ||
| privateKey.length === 66 && privateKey.startsWith(SECP256K1_PREFIX) | ||
| ? privateKey.slice(2) | ||
| : privateKey | ||
|
|
||
| const buffer = Buffer.from(normalizedPrivateKey, 'hex') | ||
|
|
||
| const publicKey = bytesToHex(nobleSecp256k1.getPublicKey(buffer, true)) | ||
| return { privateKey, publicKey } | ||
| }, | ||
|
|
Contributor
There was a problem hiding this comment.
Avoid Node.js Buffer; use isomorphic hexToBytes and validate hex
Using Buffer breaks browser environments. This package is isomorphic; prefer hexToBytes. Also add a strict hex check to fail fast on malformed input.
Apply this diff within the selected range:
} {
assert.ok(
(privateKey.length === 66 && privateKey.startsWith(SECP256K1_PREFIX)) ||
privateKey.length === 64,
'Invalid private key length or format',
)
- const normalizedPrivateKey =
+ const normalizedPrivateKey =
privateKey.length === 66 && privateKey.startsWith(SECP256K1_PREFIX)
? privateKey.slice(2)
: privateKey
-
- const buffer = Buffer.from(normalizedPrivateKey, 'hex')
-
- const publicKey = bytesToHex(nobleSecp256k1.getPublicKey(buffer, true))
+ assert.ok(
+ /^[0-9a-fA-F]{64}$/.test(normalizedPrivateKey),
+ 'private key must be 32-byte hex',
+ )
+ const privBytes = hexToBytes(normalizedPrivateKey)
+ const publicKey = bytesToHex(nobleSecp256k1.getPublicKey(privBytes, true))
return { privateKey, publicKey }
},And update the import (outside this hunk):
// change this:
import { bytesToHex } from '@xrplf/isomorphic/utils'
// to this:
import { bytesToHex, hexToBytes } from '@xrplf/isomorphic/utils'🤖 Prompt for AI Agents
In packages/ripple-keypairs/src/signing-schemes/secp256k1/index.ts around lines
33 to 52, the code uses Node's Buffer to decode a hex private key and lacks
strict hex validation; replace Buffer.from(...) with the isomorphic
hexToBytes(privateHex) function (and update the import to include hexToBytes),
and add a strict hex format check (e.g., /^[0-9a-fA-F]+$/ with expected length
64) to fail fast on malformed input before converting and deriving the public
key.
Comment on lines
+177
to
+191
| /** | ||
| * Derives a wallet from a private key. | ||
| * | ||
| * @param privateKey - A string used to generate a keypair (publicKey/privateKey) to derive a wallet. | ||
| * @returns A Wallet derived from a private key. | ||
| * | ||
| * @throws ValidationError if private key is not a valid string | ||
| */ | ||
| public static fromPrivateKey(privateKey: string): Wallet { | ||
| if (!privateKey || typeof privateKey !== 'string') { | ||
| throw new ValidationError('privateKey must be a non-empty string') | ||
| } | ||
| return new Wallet(derivePublicKey(privateKey), privateKey) | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden validation and normalize error surface
- Ambiguous input: ed25519 requires an ED-prefixed private key for downstream APIs. Accepting raw 64-hex for ed25519 leads to mis-detection as secp256k1 and subtle bugs. At minimum, make the error explicit.
- Wrap ripple-keypairs errors in ValidationError to present a consistent API surface.
Apply this diff:
public static fromPrivateKey(privateKey: string): Wallet {
- if (!privateKey || typeof privateKey !== 'string') {
- throw new ValidationError('privateKey must be a non-empty string')
- }
- return new Wallet(derivePublicKey(privateKey), privateKey)
+ if (typeof privateKey !== 'string' || privateKey.length === 0) {
+ throw new ValidationError('privateKey must be a non-empty string')
+ }
+ try {
+ const publicKey = derivePublicKey(privateKey)
+ return new Wallet(publicKey, privateKey)
+ } catch (e) {
+ // Note: ed25519 private keys must be 'ED' prefixed (66 hex chars).
+ // secp256k1 private keys may be '00' prefixed (66 hex chars) or raw 64-hex.
+ throw new ValidationError(
+ `Invalid or unsupported privateKey format${
+ e instanceof Error ? `: ${e.message}` : ''
+ }`,
+ )
+ }
}Also update the JSDoc to note prefix requirements for each algorithm.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * Derives a wallet from a private key. | |
| * | |
| * @param privateKey - A string used to generate a keypair (publicKey/privateKey) to derive a wallet. | |
| * @returns A Wallet derived from a private key. | |
| * | |
| * @throws ValidationError if private key is not a valid string | |
| */ | |
| public static fromPrivateKey(privateKey: string): Wallet { | |
| if (!privateKey || typeof privateKey !== 'string') { | |
| throw new ValidationError('privateKey must be a non-empty string') | |
| } | |
| return new Wallet(derivePublicKey(privateKey), privateKey) | |
| } | |
| /** | |
| * Derives a wallet from a private key. | |
| * | |
| * @param privateKey - A string used to generate a keypair (publicKey/privateKey) to derive a wallet. | |
| * @returns A Wallet derived from a private key. | |
| * | |
| * @throws ValidationError if private key is not a valid string | |
| */ | |
| public static fromPrivateKey(privateKey: string): Wallet { | |
| if (typeof privateKey !== 'string' || privateKey.length === 0) { | |
| throw new ValidationError('privateKey must be a non-empty string') | |
| } | |
| try { | |
| const publicKey = derivePublicKey(privateKey) | |
| return new Wallet(publicKey, privateKey) | |
| } catch (e) { | |
| // Note: ed25519 private keys must be 'ED' prefixed (66 hex chars). | |
| // secp256k1 private keys may be '00' prefixed (66 hex chars) or raw 64-hex. | |
| throw new ValidationError( | |
| `Invalid or unsupported privateKey format${ | |
| e instanceof Error ? `: ${e.message}` : '' | |
| }`, | |
| ) | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/xrpl/src/Wallet/index.ts around lines 177 to 191, the fromPrivateKey
method currently accepts ambiguous inputs and can mis-detect ed25519 keys;
update validation to require and validate algorithm-specific formats (e.g.
require ED-prefixed private keys for ed25519 and explicit, validated hex format
for secp256k1), throw a ValidationError with an explicit message when the
prefix/format is wrong, and wrap any underlying ripple-keypairs errors in a
ValidationError so the API surface is consistent; also update the JSDoc to
document the prefix/format requirements for each algorithm.
Comment on lines
+440
to
+484
| describe('from PrivateKey', function () { | ||
| describe('using secp256k1 private key', function () { | ||
| const mockWallet_secp256k1 = { | ||
| address: 'rhvh5SrgBL5V8oeV9EpDuVszeJSSCEkbPc', | ||
| publicKey: | ||
| '030E58CDD076E798C84755590AAF6237CA8FAE821070A59F648B517A30DC6F589D', | ||
| privateKey: | ||
| '00141BA006D3363D2FB2785E8DF4E44D3A49908780CB4FB51F6D217C08C021429F', | ||
| } | ||
|
|
||
| it('derive keypair from private key', function () { | ||
| const wallet = Wallet.fromPrivateKey(mockWallet_secp256k1.privateKey) | ||
| assert.equal(wallet.address, mockWallet_secp256k1.address) | ||
| assert.equal(wallet.publicKey, mockWallet_secp256k1.publicKey) | ||
| }) | ||
|
|
||
| it('throws error for malformed secp256k1 private key', function () { | ||
| assert.throws(() => | ||
| Wallet.fromPrivateKey(mockWallet_secp256k1.privateKey.slice(0, 10)), | ||
| ) | ||
| }) | ||
| }) | ||
|
|
||
| describe('using ed25519 private key', function () { | ||
| const mockWallet_ed25519 = { | ||
| address: 'rszPLM97iS8mFTndKQNexGhY1N9ionLVAx', | ||
| publicKey: | ||
| 'EDFD5C3E305FDEB97A89FC39ED333A710A7ED35E3471443C4989F9E3B8F023488D', | ||
| privateKey: | ||
| 'EDDA8694C151CE30E8A2C91884E26BC11A75514E3A27EE6CE4615FABA3DCBE1429', | ||
| } | ||
| it('derive keypair from private key', function () { | ||
| const wallet = Wallet.fromPrivateKey(mockWallet_ed25519.privateKey) | ||
| assert.equal(wallet.address, mockWallet_ed25519.address) | ||
| assert.equal(wallet.publicKey, mockWallet_ed25519.publicKey) | ||
| }) | ||
|
|
||
| it('throws error for malformed ed25519 private key', function () { | ||
| assert.throws(() => | ||
| Wallet.fromPrivateKey(mockWallet_ed25519.privateKey.slice(0, 10)), | ||
| ) | ||
| }) | ||
| }) | ||
| }) | ||
|
|
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden fromPrivateKey tests: fix suite name, improve consistency, and extend coverage for accepted inputs and invalid cases
Great addition; this validates the new API across both curves. A few improvements will make this suite more robust and consistent with the rest of the file:
- Consistency/naming: use fromPrivateKey (no space) to match method name; use camelCase for mock constants; align test titles with “derives a wallet…” phrasing used elsewhere.
- Assertions: also assert privateKey is preserved, classicAddress equals address, and seed is undefined when constructed from a privateKey.
- Coverage: add tests for accepted input forms:
- secp256k1 64-char hex without the leading "00" prefix
- ed25519 without the "ED" prefix
- Negative cases: add tests for empty string and non-hex input.
Apply this diff within this block:
- describe('from PrivateKey', function () {
+ describe('fromPrivateKey', function () {
describe('using secp256k1 private key', function () {
- const mockWallet_secp256k1 = {
+ const mockWalletSecp256k1 = {
address: 'rhvh5SrgBL5V8oeV9EpDuVszeJSSCEkbPc',
publicKey:
'030E58CDD076E798C84755590AAF6237CA8FAE821070A59F648B517A30DC6F589D',
privateKey:
'00141BA006D3363D2FB2785E8DF4E44D3A49908780CB4FB51F6D217C08C021429F',
}
- it('derive keypair from private key', function () {
- const wallet = Wallet.fromPrivateKey(mockWallet_secp256k1.privateKey)
- assert.equal(wallet.address, mockWallet_secp256k1.address)
- assert.equal(wallet.publicKey, mockWallet_secp256k1.publicKey)
+ it('derives a wallet from private key', function () {
+ const wallet = Wallet.fromPrivateKey(mockWalletSecp256k1.privateKey)
+ assert.equal(wallet.address, mockWalletSecp256k1.address)
+ assert.equal(wallet.publicKey, mockWalletSecp256k1.publicKey)
+ assert.equal(wallet.privateKey, mockWalletSecp256k1.privateKey)
+ assert.equal(wallet.classicAddress, wallet.address)
+ assert.isUndefined((wallet as unknown as { seed?: string }).seed)
})
+ it('accepts 64-char secp256k1 private key without 00 prefix', function () {
+ const unprefixed = mockWalletSecp256k1.privateKey.slice(2)
+ const wallet = Wallet.fromPrivateKey(unprefixed)
+ assert.equal(wallet.address, mockWalletSecp256k1.address)
+ assert.equal(wallet.publicKey, mockWalletSecp256k1.publicKey)
+ })
+
it('throws error for malformed secp256k1 private key', function () {
assert.throws(() =>
- Wallet.fromPrivateKey(mockWallet_secp256k1.privateKey.slice(0, 10)),
+ Wallet.fromPrivateKey(mockWalletSecp256k1.privateKey.slice(0, 10)),
)
})
})
describe('using ed25519 private key', function () {
- const mockWallet_ed25519 = {
+ const mockWalletEd25519 = {
address: 'rszPLM97iS8mFTndKQNexGhY1N9ionLVAx',
publicKey:
'EDFD5C3E305FDEB97A89FC39ED333A710A7ED35E3471443C4989F9E3B8F023488D',
privateKey:
'EDDA8694C151CE30E8A2C91884E26BC11A75514E3A27EE6CE4615FABA3DCBE1429',
}
- it('derive keypair from private key', function () {
- const wallet = Wallet.fromPrivateKey(mockWallet_ed25519.privateKey)
- assert.equal(wallet.address, mockWallet_ed25519.address)
- assert.equal(wallet.publicKey, mockWallet_ed25519.publicKey)
+ it('derives a wallet from private key', function () {
+ const wallet = Wallet.fromPrivateKey(mockWalletEd25519.privateKey)
+ assert.equal(wallet.address, mockWalletEd25519.address)
+ assert.equal(wallet.publicKey, mockWalletEd25519.publicKey)
+ assert.equal(wallet.privateKey, mockWalletEd25519.privateKey)
+ assert.equal(wallet.classicAddress, wallet.address)
+ assert.isUndefined((wallet as unknown as { seed?: string }).seed)
})
+ it('accepts ed25519 private key without ED prefix', function () {
+ const unprefixed = mockWalletEd25519.privateKey.slice(2)
+ const wallet = Wallet.fromPrivateKey(unprefixed)
+ assert.equal(wallet.address, mockWalletEd25519.address)
+ assert.equal(wallet.publicKey, mockWalletEd25519.publicKey)
+ })
it('throws error for malformed ed25519 private key', function () {
assert.throws(() =>
- Wallet.fromPrivateKey(mockWallet_ed25519.privateKey.slice(0, 10)),
+ Wallet.fromPrivateKey(mockWalletEd25519.privateKey.slice(0, 10)),
)
})
})
+
+ it('throws error for empty private key', function () {
+ assert.throws(() => Wallet.fromPrivateKey(''))
+ })
+
+ it('throws error for non-hex private key', function () {
+ assert.throws(() => Wallet.fromPrivateKey('not-hex'))
+ })
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| describe('from PrivateKey', function () { | |
| describe('using secp256k1 private key', function () { | |
| const mockWallet_secp256k1 = { | |
| address: 'rhvh5SrgBL5V8oeV9EpDuVszeJSSCEkbPc', | |
| publicKey: | |
| '030E58CDD076E798C84755590AAF6237CA8FAE821070A59F648B517A30DC6F589D', | |
| privateKey: | |
| '00141BA006D3363D2FB2785E8DF4E44D3A49908780CB4FB51F6D217C08C021429F', | |
| } | |
| it('derive keypair from private key', function () { | |
| const wallet = Wallet.fromPrivateKey(mockWallet_secp256k1.privateKey) | |
| assert.equal(wallet.address, mockWallet_secp256k1.address) | |
| assert.equal(wallet.publicKey, mockWallet_secp256k1.publicKey) | |
| }) | |
| it('throws error for malformed secp256k1 private key', function () { | |
| assert.throws(() => | |
| Wallet.fromPrivateKey(mockWallet_secp256k1.privateKey.slice(0, 10)), | |
| ) | |
| }) | |
| }) | |
| describe('using ed25519 private key', function () { | |
| const mockWallet_ed25519 = { | |
| address: 'rszPLM97iS8mFTndKQNexGhY1N9ionLVAx', | |
| publicKey: | |
| 'EDFD5C3E305FDEB97A89FC39ED333A710A7ED35E3471443C4989F9E3B8F023488D', | |
| privateKey: | |
| 'EDDA8694C151CE30E8A2C91884E26BC11A75514E3A27EE6CE4615FABA3DCBE1429', | |
| } | |
| it('derive keypair from private key', function () { | |
| const wallet = Wallet.fromPrivateKey(mockWallet_ed25519.privateKey) | |
| assert.equal(wallet.address, mockWallet_ed25519.address) | |
| assert.equal(wallet.publicKey, mockWallet_ed25519.publicKey) | |
| }) | |
| it('throws error for malformed ed25519 private key', function () { | |
| assert.throws(() => | |
| Wallet.fromPrivateKey(mockWallet_ed25519.privateKey.slice(0, 10)), | |
| ) | |
| }) | |
| }) | |
| }) | |
| describe('fromPrivateKey', function () { | |
| describe('using secp256k1 private key', function () { | |
| const mockWalletSecp256k1 = { | |
| address: 'rhvh5SrgBL5V8oeV9EpDuVszeJSSCEkbPc', | |
| publicKey: | |
| '030E58CDD076E798C84755590AAF6237CA8FAE821070A59F648B517A30DC6F589D', | |
| privateKey: | |
| '00141BA006D3363D2FB2785E8DF4E44D3A49908780CB4FB51F6D217C08C021429F', | |
| } | |
| it('derives a wallet from private key', function () { | |
| const wallet = Wallet.fromPrivateKey(mockWalletSecp256k1.privateKey) | |
| assert.equal(wallet.address, mockWalletSecp256k1.address) | |
| assert.equal(wallet.publicKey, mockWalletSecp256k1.publicKey) | |
| assert.equal(wallet.privateKey, mockWalletSecp256k1.privateKey) | |
| assert.equal(wallet.classicAddress, wallet.address) | |
| assert.isUndefined((wallet as unknown as { seed?: string }).seed) | |
| }) | |
| it('accepts 64-char secp256k1 private key without 00 prefix', function () { | |
| const unprefixed = mockWalletSecp256k1.privateKey.slice(2) | |
| const wallet = Wallet.fromPrivateKey(unprefixed) | |
| assert.equal(wallet.address, mockWalletSecp256k1.address) | |
| assert.equal(wallet.publicKey, mockWalletSecp256k1.publicKey) | |
| }) | |
| it('throws error for malformed secp256k1 private key', function () { | |
| assert.throws(() => | |
| Wallet.fromPrivateKey(mockWalletSecp256k1.privateKey.slice(0, 10)), | |
| ) | |
| }) | |
| }) | |
| describe('using ed25519 private key', function () { | |
| const mockWalletEd25519 = { | |
| address: 'rszPLM97iS8mFTndKQNexGhY1N9ionLVAx', | |
| publicKey: | |
| 'EDFD5C3E305FDEB97A89FC39ED333A710A7ED35E3471443C4989F9E3B8F023488D', | |
| privateKey: | |
| 'EDDA8694C151CE30E8A2C91884E26BC11A75514E3A27EE6CE4615FABA3DCBE1429', | |
| } | |
| it('derives a wallet from private key', function () { | |
| const wallet = Wallet.fromPrivateKey(mockWalletEd25519.privateKey) | |
| assert.equal(wallet.address, mockWalletEd25519.address) | |
| assert.equal(wallet.publicKey, mockWalletEd25519.publicKey) | |
| assert.equal(wallet.privateKey, mockWalletEd25519.privateKey) | |
| assert.equal(wallet.classicAddress, wallet.address) | |
| assert.isUndefined((wallet as unknown as { seed?: string }).seed) | |
| }) | |
| it('accepts ed25519 private key without ED prefix', function () { | |
| const unprefixed = mockWalletEd25519.privateKey.slice(2) | |
| const wallet = Wallet.fromPrivateKey(unprefixed) | |
| assert.equal(wallet.address, mockWalletEd25519.address) | |
| assert.equal(wallet.publicKey, mockWalletEd25519.publicKey) | |
| }) | |
| it('throws error for malformed ed25519 private key', function () { | |
| assert.throws(() => | |
| Wallet.fromPrivateKey(mockWalletEd25519.privateKey.slice(0, 10)), | |
| ) | |
| }) | |
| }) | |
| it('throws error for empty private key', function () { | |
| assert.throws(() => Wallet.fromPrivateKey('')) | |
| }) | |
| it('throws error for non-hex private key', function () { | |
| assert.throws(() => Wallet.fromPrivateKey('not-hex')) | |
| }) | |
| }) |
Collaborator
There was a problem hiding this comment.
@interc0der The following two test cases are useful additions: accepts ed25519/secp256k1 private key without ED/00 prefix
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
ckeshava
previously approved these changes
Aug 14, 2025
pdp2121
previously approved these changes
Oct 20, 2025
Collaborator
pdp2121
left a comment
pdp2121
left a comment
There was a problem hiding this comment.
LGTM. Can you add the change to the HISTORY.md file?
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/xrpl/src/Wallet/index.ts (1)
169-176:⚠️ Potential issue | 🟠 MajorNormalize
fromPrivateKeyerrors toValidationErrorand document accepted formats.Line 175 directly delegates to
derivePublicKey, which can throw non-ValidationErrorexceptions (assert/genericError). That makesfromPrivateKey’s error surface inconsistent with its own@throwscontract in Line 169-170. Also, accepted key formats/prefix expectations are not explicit here.🛡️ Proposed hardening
/** * Derives a wallet from a private key. * * `@param` privateKey - A string used to generate a keypair (publicKey/privateKey) to derive a wallet. * `@returns` A Wallet derived from a private key. * - * `@throws` ValidationError if private key is not a valid string + * `@throws` ValidationError if the private key is empty, invalid, or unsupported. */ public static fromPrivateKey(privateKey: string): Wallet { - if (!privateKey || typeof privateKey !== 'string') { + if (typeof privateKey !== 'string' || privateKey.length === 0) { throw new ValidationError('privateKey must be a non-empty string') } - return new Wallet(derivePublicKey(privateKey), privateKey) + try { + const publicKey = derivePublicKey(privateKey) + return new Wallet(publicKey, privateKey) + } catch (error) { + throw new ValidationError( + `Invalid or unsupported privateKey format${ + error instanceof Error ? `: ${error.message}` : '' + }`, + ) + } }#!/bin/bash # Verify current fromPrivateKey implementation does not normalize downstream errors. rg -n -A20 -B5 "public static fromPrivateKey\\(" packages/xrpl/src/Wallet/index.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/Wallet/index.ts` around lines 169 - 176, fromPrivateKey currently lets derivePublicKey throw non-ValidationError errors; wrap the derivePublicKey call in a try/catch inside Wallet.fromPrivateKey and rethrow any error as a ValidationError so the method honors its `@throws` contract (include the original error message in the ValidationError). Also expand the JSDoc for fromPrivateKey to explicitly state accepted private key formats/prefixes (e.g., type/string expectations and any XRPL prefix requirements) so callers know valid input. Target symbols: Wallet.fromPrivateKey, derivePublicKey, ValidationError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/xrpl/src/Wallet/index.ts`:
- Around line 169-176: fromPrivateKey currently lets derivePublicKey throw
non-ValidationError errors; wrap the derivePublicKey call in a try/catch inside
Wallet.fromPrivateKey and rethrow any error as a ValidationError so the method
honors its `@throws` contract (include the original error message in the
ValidationError). Also expand the JSDoc for fromPrivateKey to explicitly state
accepted private key formats/prefixes (e.g., type/string expectations and any
XRPL prefix requirements) so callers know valid input. Target symbols:
Wallet.fromPrivateKey, derivePublicKey, ValidationError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ae1f6ecf-d07a-4844-933a-17f948740905
📒 Files selected for processing (1)
packages/xrpl/src/Wallet/index.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ripple-keypairs/HISTORY.md`:
- Around line 5-9: Remove the duplicated `derivePublicKey` changelog entry from
the older `2.0.0 (2024-02-01)` release and ensure the feature is only listed
under `2.0.1 (2026-03-06)` (the correct release that introduced it); also remove
or convert the dated release header placed directly under `Unreleased` into an
undated "Unreleased" section so pre-release entries aren’t ambiguous. Locate and
edit the `derivePublicKey` bullet and the `2.0.0`/`2.0.1` headers in HISTORY.md
to keep a single authoritative entry and tidy the release headings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4b8ddd12-72b3-4dc7-9e49-566a16bce106
📒 Files selected for processing (2)
packages/ripple-keypairs/HISTORY.mdpackages/ripple-keypairs/package.json
✅ Files skipped from review due to trivial changes (1)
- packages/ripple-keypairs/package.json
Comment on lines
+5
to
+9
| ## 2.0.1 (2026-03-06) | ||
|
|
||
| ### Non-Breaking Changes | ||
| * Add `derivePublicKey` method to derive the keypair from an arbitrary private key | ||
|
|
Contributor
There was a problem hiding this comment.
Changelog versioning is inconsistent and duplicates the same feature entry.
derivePublicKey is listed under both 2.0.1 (2026-03-06) and 2.0.0 (2024-02-01). Since 2.0.0 is already dated February 1, 2024, this feature should not be backfilled there. Also, keeping a dated release header directly under Unreleased is ambiguous before release.
Suggested HISTORY.md cleanup
## Unreleased
-## 2.0.1 (2026-03-06)
+## 2.1.0 (2026-03-06)
### Non-Breaking Changes
* Add `derivePublicKey` method to derive the keypair from an arbitrary private key
@@
### Non-Breaking Changes
* Remove `brorand` as a dependency and use `@xrplf/isomorphic` instead.
* Eliminates 4 runtime dependencies: `base-x`, `base64-js`, `buffer`, and `ieee754`.
-* Add `derivePublicKey` method to derive the keypair from an arbitrary private keyAlso applies to: 26-26
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ripple-keypairs/HISTORY.md` around lines 5 - 9, Remove the
duplicated `derivePublicKey` changelog entry from the older `2.0.0 (2024-02-01)`
release and ensure the feature is only listed under `2.0.1 (2026-03-06)` (the
correct release that introduced it); also remove or convert the dated release
header placed directly under `Unreleased` into an undated "Unreleased" section
so pre-release entries aren’t ambiguous. Locate and edit the `derivePublicKey`
bullet and the `2.0.0`/`2.0.1` headers in HISTORY.md to keep a single
authoritative entry and tidy the release headings.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Continuation of previous pull request: #3059
High Level Overview of Change
As there are more wallets natively supporting the XRP Ledger, private keys are being passed around in different context and there should be a method in this codebase to derive a wallet from a private key. Since we have (2) algorithms and this package correctly handles both, this is a necessary addition to the package and offers immense value.
Context of Change
Type of Change
Did you update HISTORY.md?
Test Plan